[SPARK-20121][SQL] simplify NullPropagation with NullIntolerant by cloud-fan · Pull Request #17450 · apache/spark

cloud-fan · 2017-03-28T05:59:11Z

What changes were proposed in this pull request?

Instead of iterating all expressions that can return null for null inputs, we can just check NullIntolerant.

How was this patch tested?

existing tests

cloud-fan · 2017-03-28T05:59:29Z

cc @gatorsmile

SparkQA · 2017-03-28T06:02:32Z

Test build #75298 has started for PR 17450 at commit dfb70fb.

gatorsmile · 2017-03-28T15:16:13Z

retest this please

SparkQA · 2017-03-28T17:25:39Z

Test build #75318 has finished for PR 17450 at commit dfb70fb.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
abstract class BinaryArithmetic extends BinaryOperator with NullIntolerant
case class Add(left: Expression, right: Expression) extends BinaryArithmetic
case class Subtract(left: Expression, right: Expression) extends BinaryArithmetic
case class Multiply(left: Expression, right: Expression) extends BinaryArithmetic
case class Divide(left: Expression, right: Expression) extends BinaryArithmetic
case class Remainder(left: Expression, right: Expression) extends BinaryArithmetic
case class Pmod(left: Expression, right: Expression) extends BinaryArithmetic
case class Like(left: Expression, right: Expression) extends StringRegexExpression
case class RLike(left: Expression, right: Expression) extends StringRegexExpression
case class Contains(left: Expression, right: Expression) extends StringPredicate
case class StartsWith(left: Expression, right: Expression) extends StringPredicate
case class EndsWith(left: Expression, right: Expression) extends StringPredicate

nsyca · 2017-03-28T19:44:33Z

 // scalastyle:on line.size.limit
 case class Substring(str: Expression, pos: Expression, len: Expression)
-  extends TernaryExpression with ImplicitCastInputTypes {
+  extends TernaryExpression with ImplicitCastInputTypes with NullIntolerant {


Is the function SUBSTRING null-intolerant? What is the return value if str is a null value?

The result can be null; if any argument is null, the result is the null value.

Ref: https://www.ibm.com/support/knowledgecenter/en/SSEPEK_10.0.0/sqlref/src/tpc/db2z_bif_substr.html

I might be confused with the terminologies: NullIntolerant expression versus "null-intolerant predicate". But if SUBSTRING is marked null-intolerant expression, why do we not mark the class of string functions such as STARTSWITH, etc. the same way? Am I missing anything here?

Yes, we should mark NullIntolerant to the other expressions, if possible, and also update the document.

@nsyca If you have a bandwidth, could you please review all the expressions and see whether they can be marked as NullIntolerant?

You can check the impl of these expressions and compare them with the corresponding ones in the other RDBMS. Thanks!

Below is a ref PR you can use: https://github.com/apache/spark/pull/15850/files. You can continue my work if you want.

I will certainly take a look. On a second thought, since "most" of the SQL functions are null-intolerant, isn't easier to mark only functions that are null-tolerant such as ISNOTNULL? I am just pitching an idea here, not indicating we should abandon this PR.

At the beginning, when we introduce NullIntolerant , @marmbrus said we should do it more carefully. We prefer to using the white-list solution for avoiding hidden bugs. Marking it NullIntolerant if and only if we are ensure that they are null intolerant. Thus, when we doing it, we should also add the corresponding test cases and documents.

gatorsmile · 2017-03-28T20:32:08Z

      // Literal(null)
      case In(Literal(null, _), list) => Literal.create(null, BooleanType)

+      // Put exceptional cases above if any


Nit: Attribute is also NullIntolerant Maybe add a comment?
Non-leaf NullIntolerant expressions will return null, if at least one of its children is a null literal.

gatorsmile · 2017-03-28T20:32:47Z

Great! LGTM except a minor comment.

viirya · 2017-03-29T07:32:11Z

-trait StringPredicate extends Predicate with ImplicitCastInputTypes {
-  self: BinaryExpression =>
+abstract class StringPredicate extends BinaryExpression
+  with Predicate with ImplicitCastInputTypes {


Missing with NullIntolerant here?

This PR is just to simplify the existing rule NullPropagation.

See above StringRegexExpression, similar to it, in order to simplify the NullPropagation, we need to add NullIntolerant, so it can propagate null value...

I finally got your point. StringPredicate is used for inferring the null constants in the rule NullPropagation. Thus, we should mark it as NullIntolerant .

viirya · 2017-03-30T14:30:47Z

LGTM

SparkQA · 2017-03-30T16:03:03Z

Test build #75390 has finished for PR 17450 at commit 63287ef.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-03-30T16:04:43Z

retest this please

SparkQA · 2017-03-30T18:19:36Z

Test build #75392 has finished for PR 17450 at commit 63287ef.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-03-30T20:35:22Z

Thanks! Merging to master.

simplify NullPropagation with NullIntolerant

dfb70fb

nsyca reviewed Mar 28, 2017

View reviewed changes

gatorsmile reviewed Mar 28, 2017

View reviewed changes

viirya reviewed Mar 29, 2017

View reviewed changes

address comments

63287ef

asfgit closed this in c734fc5 Mar 30, 2017

Conversation

cloud-fan commented Mar 28, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

cloud-fan commented Mar 28, 2017

Uh oh!

SparkQA commented Mar 28, 2017

Uh oh!

gatorsmile commented Mar 28, 2017

Uh oh!

SparkQA commented Mar 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nsyca Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Mar 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya commented Mar 30, 2017

Uh oh!

SparkQA commented Mar 30, 2017

Uh oh!

gatorsmile commented Mar 30, 2017

Uh oh!

SparkQA commented Mar 30, 2017

Uh oh!

gatorsmile commented Mar 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nsyca Mar 29, 2017 •

edited

Loading

gatorsmile Mar 29, 2017 •

edited

Loading

gatorsmile Mar 29, 2017 •

edited

Loading

gatorsmile Mar 28, 2017 •

edited

Loading